Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update and clarify documentation comments #5206

Merged
merged 16 commits into from
Sep 23, 2024

Conversation

cairoeth
Copy link
Contributor

@cairoeth cairoeth commented Sep 16, 2024

Fixes N-06, N-08, N-10 and N-13

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Sep 16, 2024

⚠️ No Changeset found

Latest commit: 36b61d1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

cairoeth and others added 2 commits September 18, 2024 17:58
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
contracts/utils/cryptography/P256.sol Outdated Show resolved Hide resolved
@@ -30,7 +30,7 @@ library RSA {
* method described in https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2[section 8.2.2 of RFC8017].
*
* IMPORTANT: Although this function allows for it, using n of length 1024 bits is considered unsafe.
* Consider using at least 2048 bits.
* Consider using at least 2048 bits. Additionally, this function only supports SHA256 as the hash function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of removing this note and rename the function to pkcs1Sha256 just as its bytes counterpart. That was the original naming iiurc but I proposed renaming it while ignoring the AlgorithmIdentifier is included during signature creation

@ernestognw
Copy link
Member

According to N-10, we should enforce 2048 bits in RSA. I updated accordingly since I do think we should enforce it.

ernestognw
ernestognw previously approved these changes Sep 18, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this file is not ok. The entier point is to take an unmodified reference (standard) and using it for tests. If any entry in this file causes an issue, the parsing/filtering should take care of that in a predictable way, but this should not change.

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment explaining how rsa key are generated using openssl (in the openssl test).

LGTM

@ernestognw ernestognw merged commit 2f0bc58 into OpenZeppelin:master Sep 23, 2024
21 checks passed
@GIgako19929
Copy link

``

@GIgako19929
Copy link

@ @GIgako19929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants